Remove `--check-cfg` checking of command line `--cfg` args #117522

Urgau · 2023-11-02T17:20:09Z

Back in #100574 we added to the unexpected_cfgs lint the checking of --cfg CLI arguments and emitted unexpected names and values for them.

The implementation works as expected, but it's usability in particular when using it in combination with Cargo+RUSTFLAGS as people who set RUSTFLAGS=--cfg=tokio_unstable (or whatever) have unexpected_cfgs warnings on all of their crates is debatable. ~~To fix this issue this PR proposes that we split the CLI argument checking into it's own separate allow-by-default lint: unexpected_cli_cfgs.~~

~~This has the advantage of letting people who want CLI warnings have them (although not by default anymore), while still linting on every unexpected cfg name and values in the code.~~

After some discussion with the Cargo team (Zulip thread) and member of the compiler team (see below), I propose that we follow the suggestion from @epage: never check --cfg arguments, but still reserve us the possibility to do it later.

We would still lint on unexpected cfgs found in the source code no matter the --cfg args passed. This mean reverting #100574 but NOT #99519.

r? @petrochenkov

compiler/rustc_lint/src/builtin.rs

petrochenkov · 2023-11-07T14:42:56Z

I don't think this a right thing to do.

Why should a specific lint in rustc compensate for deficiencies in cargo (no RUSTFLAGS or --cfg support targeted at specific crates) or libraries (use of --cfg instead of features despite cargo's poor support for it)?

I think it's fine for people using such a large hammer as RUSTFLAGS=--cfg=tokio_unstable to use -A unexpected_cfgs as well.

Urgau · 2023-11-07T16:52:58Z

I personally see this as a short/medium term workaround until Cargo fixes the problem on their end.
In particular Pre-RFC: Mutually-excusive, global features would help tremendously.
After Cargo fixes the problem we would change the lint back to warn-by-default.

I think it's fine for people using such a large hammer as RUSTFLAGS=--cfg=tokio_unstable to use -A unexpected_cfgs as well.

Member of the Cargo team have expressed their discomfort and even objection to this.

Why should a specific lint in rustc compensate for deficiencies in cargo?

I agree that from the point of view of rustc it's not great. One possibility could be to make the lint warn-by-default and have Cargo always pass (for now) -Aunexpected_cli_cfgs. Would this remove your concern?

cc @epage (who have strong opinion on this)
@rustbot ready

weihanglo · 2023-11-12T05:32:38Z

I personally see this as a short/medium term workaround until Cargo fixes the problem on their end.

Could you elaborate why we need this short term workaround when the feature is still in nightly?

Urgau · 2023-11-12T10:35:08Z

Could you elaborate why we need this short term workaround when the feature is still in nightly?

The feature is currently in nightly but I want to stabilize it in the near future and as far as I know this issue with RUSTFLAGS is the biggest remaining concern.

To be more precise separating in two different lints lets us have warnings for every unexpected cfgs in the code and in the cli, but with the possibility to individually disable one or another without disturbing the other one.

weihanglo · 2023-11-12T14:16:26Z

See also the discussion on Zulip: https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/check-cfg.20and.20RUSTFLAGS.20interaction

Urgau · 2023-11-16T09:57:52Z

We (I and members of the Cargo team) had some discussion yesterday about --check-cfg, you find the summary of that discussion (by @epage, thanks) at #82450 (comment).

One proposed solution that was suggested by @epage would be to never check --cfg arguments, in contrast to dividing the check under a different lint (what this PR currently propose), but to still reserve us the possibility to do it later.

This would still mean that we would lint on unexpected cfgs found in the source code no matter the --cfg args passed (ie. reverting #100574 but NOT #99519).

@petrochenkov what do you think about this?

petrochenkov · 2023-11-17T16:41:48Z

@Urgau @epage
Is RUSTFLAGS still in the environment when rustc is run, or cargo removes it from environment for rustc?
If it is still in the environment we could suppress the warning for all --cfgs found in RUSTFLAGS specifically.

Urgau · 2023-11-17T16:53:15Z

Is RUSTFLAGS still in the environment when rustc is run, or cargo removes it from environment for rustc?

It is still in the environment, but the rust flags can also be set from the .cargo/config.toml file:

[target.<triple>]
rustflags = ["…", "…"]

and those are not included in the RUSTFLAGS env (when mixed only the env takes priority).

So I don't think that a viable option; that's also ignoring that we would have to parse the RUSTFLAGS env which doesn't seems particularly attractive to me, note to mention that Cargo maps every TOML option to an env which means that CARGO_TARGET_<target>_RUSTFLAGS is the same thing as the config file above, increasing again the things rustc need to know.

@rustbot ready

petrochenkov · 2023-11-17T19:26:19Z

Then suppressing the warning for all --cfg seems acceptable to me.

rustbot · 2023-11-18T11:33:00Z

Could not assign reviewer from: petrochenkov.
User(s) petrochenkov are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

Urgau · 2023-11-18T11:38:58Z

I've updated the PR to no longer check any --cfg args (basically reverting #100574). This PR should be ready for review.

@rustbot ready

petrochenkov · 2023-11-21T13:31:54Z

@bors r+

bors · 2023-11-21T13:31:57Z

📌 Commit a5658e6 has been approved by petrochenkov

It is now in the queue for this repository.

Rollup of 6 pull requests Successful merges: - rust-lang#116085 (rustdoc-search: add support for traits and associated types) - rust-lang#117522 (Remove `--check-cfg` checking of command line `--cfg` args) - rust-lang#118029 (Expand Miri's BorTag GC to a Provenance GC) - rust-lang#118035 (Fix early param lifetimes in generic_const_exprs) - rust-lang#118083 (Remove i686-apple-darwin cross-testing) - rust-lang#118091 (Remove now deprecated target x86_64-sun-solaris.) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#117522 - Urgau:check-cfg-cli-own-lint, r=petrochenkov Remove `--check-cfg` checking of command line `--cfg` args Back in rust-lang#100574 we added to the `unexpected_cfgs` lint the checking of `--cfg` CLI arguments and emitted unexpected names and values for them. The implementation works as expected, but it's usability in particular when using it in combination with Cargo+`RUSTFLAGS` as people who set `RUSTFLAGS=--cfg=tokio_unstable` (or whatever) have `unexpected_cfgs` warnings on all of their crates is debatable. ~~To fix this issue this PR proposes that we split the CLI argument checking into it's own separate allow-by-default lint: `unexpected_cli_cfgs`.~~ ~~This has the advantage of letting people who want CLI warnings have them (although not by default anymore), while still linting on every unexpected cfg name and values in the code.~~ After some discussion with the Cargo team ([Zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/check-cfg.20and.20RUSTFLAGS.20interaction)) and member of the compiler team (see below), I propose that we follow the suggestion from `@epage:` never check `--cfg` arguments, but still reserve us the possibility to do it later. We would still lint on unexpected cfgs found in the source code no matter the `--cfg` args passed. This mean reverting rust-lang#100574 but NOT rust-lang#99519. r? `@petrochenkov`

rustbot assigned petrochenkov Nov 2, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 2, 2023

This comment has been minimized.

Sign in to view

Urgau force-pushed the check-cfg-cli-own-lint branch from 3f48f02 to 21109fb Compare November 2, 2023 17:56

Urgau commented Nov 3, 2023

View reviewed changes

compiler/rustc_lint/src/builtin.rs Outdated Show resolved Hide resolved

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 7, 2023

epage mentioned this pull request Nov 16, 2023

Tracking Issue for RFC 3013: Checking conditional compilation at compile time #82450

Closed

3 tasks

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 17, 2023

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2023

Urgau added 2 commits November 18, 2023 12:21

Remove --check-cfg checking of --cfg args

f5023e4

Update --check-cfg tests that had old --cfg warning

a5658e6

Urgau force-pushed the check-cfg-cli-own-lint branch from 21109fb to a5658e6 Compare November 18, 2023 11:27

Urgau changed the title ~~Make --check-cfg use a different lint for CLI --cfg checking~~ Remove --check-cfg checking of command line --cfg args Nov 18, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 18, 2023

Urgau mentioned this pull request Nov 18, 2023

rustbot should not throw an error when updating a message containing a assignment rust-lang/triagebot#1750

Open

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2023

Noratrieb mentioned this pull request Nov 21, 2023

Rollup of 6 pull requests #118134

Merged

bors merged commit aff407e into rust-lang:master Nov 21, 2023
11 checks passed

rustbot added this to the 1.76.0 milestone Nov 21, 2023

Urgau deleted the check-cfg-cli-own-lint branch November 21, 2023 17:18

ehuss mentioned this pull request Dec 16, 2023

don't try to reassign a reviewer when a pr title is edited rust-lang/triagebot#1755

Merged

Urgau mentioned this pull request Feb 26, 2024

Tracking Issue for rustc --check-cfg integration rust-lang/cargo#10554

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `--check-cfg` checking of command line `--cfg` args #117522

Remove `--check-cfg` checking of command line `--cfg` args #117522

Urgau commented Nov 2, 2023 •

edited

Loading

This comment has been minimized.

petrochenkov commented Nov 7, 2023

Urgau commented Nov 7, 2023

weihanglo commented Nov 12, 2023

Urgau commented Nov 12, 2023 •

edited

Loading

weihanglo commented Nov 12, 2023

Urgau commented Nov 16, 2023 •

edited

Loading

petrochenkov commented Nov 17, 2023

Urgau commented Nov 17, 2023 •

edited

Loading

petrochenkov commented Nov 17, 2023

rustbot commented Nov 18, 2023

Urgau commented Nov 18, 2023 •

edited

Loading

petrochenkov commented Nov 21, 2023

bors commented Nov 21, 2023

Remove --check-cfg checking of command line --cfg args #117522

Remove --check-cfg checking of command line --cfg args #117522

Conversation

Urgau commented Nov 2, 2023 • edited Loading

This comment has been minimized.

petrochenkov commented Nov 7, 2023

Urgau commented Nov 7, 2023

weihanglo commented Nov 12, 2023

Urgau commented Nov 12, 2023 • edited Loading

weihanglo commented Nov 12, 2023

Urgau commented Nov 16, 2023 • edited Loading

petrochenkov commented Nov 17, 2023

Urgau commented Nov 17, 2023 • edited Loading

petrochenkov commented Nov 17, 2023

rustbot commented Nov 18, 2023

Urgau commented Nov 18, 2023 • edited Loading

petrochenkov commented Nov 21, 2023

bors commented Nov 21, 2023

Remove `--check-cfg` checking of command line `--cfg` args #117522

Remove `--check-cfg` checking of command line `--cfg` args #117522

Urgau commented Nov 2, 2023 •

edited

Loading

Urgau commented Nov 12, 2023 •

edited

Loading

Urgau commented Nov 16, 2023 •

edited

Loading

Urgau commented Nov 17, 2023 •

edited

Loading

Urgau commented Nov 18, 2023 •

edited

Loading